-
-
Notifications
You must be signed in to change notification settings - Fork 81
SketchException refactor #1164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
SketchException refactor #1164
Conversation
This reverts commit 105bc96.
fixes bug with selecting an error that's in a tab that isn't visible yet.
…dd-toniab docs: add toniab as a contributor for code
Update BUILD.md
…dd-toniab docs: add toniab as a contributor for doc
Properly clear EditorStatus.message with empty()
…dd-aj-m docs: add aj-m as a contributor for code
…dd-pnngocdoan docs: add pnngocdoan as a contributor for code
…dd-manoellribeiro docs: add manoellribeiro as a contributor for doc
…or-guidelines-assignment
Improved the phrasing on Jetpack Compose
…asing-jetpack Update BUILD.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @joshgiesbrecht Sorry it took me so long to take a good look at this. Thank you for taking this on and fixing this issue!
Looks good to me, I added a few comments that are mostly about style.
app/utils/build.gradle.kts
Outdated
id("java") | ||
} | ||
|
||
group = "org.example" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by "these" you meant the group and version lines, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
app/.gitignore
Outdated
@@ -1,2 +1,3 @@ | |||
bin | |||
pde.jar | |||
/utils/build/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the top level .gitignore
…ls.SketchException, except I'm missing something because on compile it's not finding it. (module / package visibility problem or somethin')
…s for now though(?)
…ketchException-refactor # Conflicts: # app/.gitignore # app/utils/build.gradle.kts
Created an
:app:utils
module, movedSketchException
to aprocessing.utils
package within:app:utils
, changed all references toSketchException
in bothjava
andapp
to useprocessing.utils.SketchException
, removed the two duplicateSketchException
classes inapp
andjava.preproc
.This currently seems to work (tested only on Linux so far, but I don't think there's anything platform-specific involved), and does solve the bug of highlighting the first error on running a sketch. Non-Java modes will not yet be using the correct SketchException class; I suspect this will be a 'soft' fail that has a similar problem to the existing bug, but I haven't tested this much yet. (Shader mode seems to be broken already?)
Since the draft PR for #1104 also creates
:app:utils
and is more complex than this PR, I'm fine with waiting until the other PR is merged, and then I can make sure this one merges cleanly before it's added in.What's the right thing to do re: the impact on non-Java modes? Should I submit PRs for those projects as well, maybe once this one's actually merged into main?